Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[18.0][ADD] account_payment_base_oca: New base module for OCA/bank-payment #1401

Draft
wants to merge 4 commits into
base: 18.0
Choose a base branch
from

Conversation

alexis-via
Copy link
Contributor

Work in progress. Expect datamodel changes.

@pedrobaeza
Copy link
Member

Why this? Why didn't you answer to my comment in the issue? I don't want data model changes if they are not justified.

@alexis-via
Copy link
Contributor Author

I'm trying to adopt the new datamodel. I'm not saying it's the best choice, but I want to try and see what difficulties I face.
I now have the full stack working: account_payment_base_oca + account_payment_order + account_banking_pain_base + account_banking_sepa_credit_transfer + account_banking_mandate + account_banking_sepa_direct_debit. So I am able to make full scenarios and see the real problems I face and try to address them.
At the moment, it's too early to list them, because I'm currently performing the tests.

@pedrobaeza
Copy link
Member

Then we are going to have 2 forks. I don't want that "datamodel", as it's very bad as explained in the migration issue.

@alexis-via alexis-via force-pushed the 18-add-account_payment_base_oca branch from 0920212 to d819284 Compare January 27, 2025 18:16
@alexis-via
Copy link
Contributor Author

alexis-via commented Jan 27, 2025

I just pushed a new version of account_payment_base_oca. In this new version, the active field of account.payment.method.line (added by account_payment_base_oca) is set to False by default. So, when a account.payment.method is added, all the account.payment.method.line automatically created by Odoo are inactive. You can active the ones you need and leave the others inactive.
You still have the option bank_account_link = "variable" as in Odoo v17-. When "variable" is selected, a M2M field alternative_journal_ids is displayed ; the possible journals are journal_id + alternative_journal_ids. journal_id acts as the default journal.
When you confirm a payment order that has a payment_method_line_id configured as "variable", Odoo will search the account.payment.method.line with the journal selected on the payment order and the same payment method and use that on the account.payment.
My first tests are positive.
I'll push the code of the other modules in a few days.

@pedrobaeza
Copy link
Member

I sincerely see it as a nonsense to have to look for "similar" things one way and the reverse because of a failed datamodel putting the data at journal level. You can't for example group by this field if you select different lines according the journal but representing the same payment mode. Let's keep our account.payment.mode model, and we can link the account.payment.method.line on the generated payment as we were doing previously.

@alexis-via alexis-via changed the title [ADD] account_payment_base_oca: New base module for OCA/bank-payment [18.0][ADD] account_payment_base_oca: New base module for OCA/bank-payment Jan 27, 2025
@alexis-via alexis-via force-pushed the 18-add-account_payment_base_oca branch from d819284 to 7871388 Compare January 27, 2025 21:54
@alexis-via
Copy link
Contributor Author

@pedrobaeza It's also a big temptation for me to keep account.payment.mode. In fact, I'm the one who introduced account.payment.mode in v9 during the Sorrento code sprint (to replace payment.mode that was dropped by Odoo SA with the removal of the module account_payment from the official addons), so it's kind of my baby and I like it.

But, in the long list of wired/sub-optimal datamodel of Odoo SA that I would love to avoid/change, this one is not a big deal. I think I found a solution that is acceptable and the balance between the advantage of being compatible with the native datamodel and the drawbacks of using the trick I propose to make the new datamodel work with bank_account_link = 'variable' tend towards the adoption of the new datamodel I think. I'll continue my tests in the coming days to make sure I don't discover any other problems and I'll publish the upper-level modules.

@alexis-via alexis-via force-pushed the 18-add-account_payment_base_oca branch from 7871388 to 9da52f0 Compare January 28, 2025 23:00
# the account.payment but it will select the (inactive) method line
# linked to the chosen journal
# TODO default=False causes problems in tests of the account module
active = fields.Boolean(default=False)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make sense to set active=False on create only when bank_account_link is variable?

@sbidoul
Copy link
Member

sbidoul commented Jan 29, 2025

This sounds promising. I also prefer using the native account.payment.method.line if that is possible.

@pedrobaeza
Copy link
Member

I insist: how it would be better to assign at partner level the payment method line linked to a specific journal, seeing all of them in the dropdown (and this active=False trick is something conflicting with the way Odoo expects to work), instead of having an agnostic payment mode?

The payment method lines are not new. They are there since ages (v14). The only new thing is that there's a many2one at partner level, but that doesn't change anything. Please don't go with this, or we will have 2 account_payment_order modules.

@pedrobaeza
Copy link
Member

Another reason spotted now for not switching from account.payment.mode: the missing logic about the mode on refunding invoices.

@sbidoul
Copy link
Member

sbidoul commented Jan 30, 2025

@pedrobaeza I think Alexis is doing a lot of very valuable work here. Let's wait a bit to see where this goes.

@pedrobaeza
Copy link
Member

No, if the foundations are incorrect, no more work should be put in here. I announce my intention on continuing with the old stack if you switch, and the model names should be preserved for the existing things according current establishment.

@sbidoul
Copy link
Member

sbidoul commented Jan 30, 2025

Sorry I don't understand your arguments, Pedro. Odoo is evolving and it seems normal to me to explore and see if we can adapt.

@pedrobaeza
Copy link
Member

The arguments are clear of the cons of switching, so I'm just stating that I would push for the previous approach and this will conflict in the model names, and it's clear that these cons can't be bypassed without doing ugly hacks and standard code overrides.

@pedrobaeza
Copy link
Member

Note that I'm the first one embracing upstream changes when they are suitable for the goal of the modules. I did the refactoring to account.payment in v14, and thanks to that, we have enjoyed 5 versions without having to touch continuously the journal entry generation, delegating that to core mechanisms.

But in this case, it has no benefit to go with a simple wrong m2o extra field that they have added in the partner, and that is so wrong in the concept. And I insist that the account.payment.method.line is not something new. It's there since ages, and we set it correctly on the generated account.payment. The only change is about the partner preferred payment mode.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants